Skip to content

added solution#293

Open
yahohulia wants to merge 19 commits into
mate-academy:masterfrom
yahohulia:develop
Open

added solution#293
yahohulia wants to merge 19 commits into
mate-academy:masterfrom
yahohulia:develop

Conversation

@yahohulia

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This implementation has multiple critical runtime bugs that will crash the application and missing required features. You must fix: (1) Missing uuidv4 import in auth.controller.js line 147 - causes crash on password reset, (2) Missing await on bcrypt.compare in auth.controller.js line 81 - login will always fail because it compares a Promise instead of a boolean, (3) Missing next parameter in profile.controller.js functions (changePassword, changeEmail, confirmEmail) - calling next() will throw ReferenceError, (4) Missing User import in profile.controller.js - User.findOne on line 79 will throw ReferenceError, (5) Token service null pointer bug in token.service.js - Token.create return value not assigned to token, causing crash on lines 9-10 when accessing token.refreshToken. Additionally: missing /change-name route as required by the task, missing 404 catch-all handler, and no activation check during login. Fix these issues before re-submission.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/controllers/auth.controller.js Outdated
Comment on lines +1 to +7
import bcrypt, { hash } from 'bcrypt';
import { User } from '../models/user.js';
import { userService } from '../services/user.service.js';
import { jwtService } from '../services/jwt.service.js';
import { ApiError } from '../exeptions/api.error.js';
import { tokenService } from '../services/token.service.js';
import { emailService } from '../services/email.service.js';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing uuidv4 import - this function is called on line 147 but the import is missing, causing a runtime crash when resetting passwords

Comment thread src/controllers/auth.controller.js Outdated
@@ -0,0 +1,205 @@
import bcrypt, { hash } from 'bcrypt';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hash is imported but never used - this is unnecessary dead code

const login = async (req, res) => {
const { email, password } = req.body;

const user = await userService.findByEmail(email);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bcrypt.compare returns a Promise but is not awaited - the password validation will always fail since it's comparing against a Promise object


const user = await userService.findByEmail(email);
const isPasswordValid = bcrypt.compare(password, user.password);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is for !email but should check for !user - if the user doesn't exist, accessing user.password on line 81 will throw a TypeError

Comment thread src/controllers/auth.controller.js Outdated
Comment on lines +1 to +6
import bcrypt, { hash } from 'bcrypt';
import { User } from '../models/user.js';
import { userService } from '../services/user.service.js';
import { jwtService } from '../services/jwt.service.js';
import { ApiError } from '../exeptions/api.error.js';
import { tokenService } from '../services/token.service.js';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing User model import - User is used on line 79 but not imported at the top of the file

Comment on lines +19 to +20
function validateEmail(value) {
if (!value) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next is called but is not defined in function parameters - the route wraps controllers with catchError which only passes req, res, next, so this should work, but verify catchError implementation passes next correctly

Comment thread src/controllers/auth.controller.js Outdated
Comment on lines +77 to +88

const login = async (req, res) => {
const { email, password } = req.body;

const user = await userService.findByEmail(email);
const isPasswordValid = bcrypt.compare(password, user.password);

if (!email || !isPasswordValid) {
throw ApiError.badRequest('Email or password is incorrect');
}

generateTokens(res, user);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements specify that users must be activated before they can log in. This implementation does not check if the user's activationToken is null before allowing login. Unactivated users will be able to authenticate.

Comment on lines +107 to +113

const userData = jwtService.verifyRefresh(refreshToken);
const token = await tokenService.getByToken(refreshToken);

if (!userData || !token) {
throw ApiError.unAuthorized();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No functionality to change name is implemented. The requirements specify that Profile page should allow users to change their name.

const { confirmToken } = req.params;

const user = await User.findOne({ where: { confirmToken } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: Missing await on bcrypt.compare() - it's an async function. Without await, isPasswordValid will be a Promise, not a boolean, causing the login check to always fail.

Comment thread src/controllers/profile.controller.js Outdated
Comment on lines +80 to +83
const user = await User.findOne({ where: { confirmToken } });

if (!user) {
throw ApiError.notFound();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: userService.findByEmail(email) can return null if no user exists, but there's no null check before accessing user.password on line 81. This will throw a TypeError.

Comment thread src/controllers/profile.controller.js Outdated
confirmToken,
'Confirm changing Email',
'Changing Email address',
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: Missing await on user.save() - Sequelize save() returns a Promise.

'Changing Email address',
);

res.status(200).send('Confirmation email sent');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: Missing await on user.save() - Sequelize save() returns a Promise.

Comment thread src/controllers/profile.controller.js Outdated
Comment on lines +77 to +87
const confirmEmail = async (req, res) => {
const { confirmToken } = req.params;

const user = await User.findOne({ where: { confirmToken } });

if (!user) {
throw ApiError.notFound();
}

if (user.confirmTokenExpiry < new Date()) {
user.confirmToken = null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUIREMENT VIOLATION: Login allows inactive users to authenticate. The task requires rejecting login if user is not yet activated and prompting them to activate their email.

return next(
ApiError.badRequest({ newPassword: 'New passwords do not match' }),
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUIREMENT VIOLATION: Registration should display password rules to users before they register. The validatePassword function only checks length but doesn't enforce other rules that should be displayed to users.

Comment on lines +1 to +79
import bcrypt from 'bcrypt';
import { v4 as uuidv4 } from 'uuid';
import { ApiError } from '../exeptions/api.error.js';
import { jwtService } from '../services/jwt.service.js';
import { userService } from '../services/user.service.js';
import { emailService } from '../services/email.service.js';

const getData = async (req, res) => {
const user = await userService.findByEmail(req.user.email);

res.status(200).send(userService.profileNormalize(user));
};

const changePassword = async (req, res) => {
const { oldPassword, newPassword1, newPassword2 } = req.body;

const user = await userService.findByEmail(req.user.email);
const isPasswordValid = await bcrypt.compare(oldPassword, user.password);

if (!isPasswordValid) {
return next(
ApiError.badRequest({ oldPassword: 'Old password is not correct' }),
);
}

if (newPassword1.trim() !== newPassword2.trim()) {
return next(
ApiError.badRequest({ newPassword: 'New passwords do not match' }),
);
}

if (newPassword1.trim() === oldPassword.trim()) {
return next(
ApiError.badRequest({
newPassword: 'New password must be different from old password',
}),
);
}

const password = await bcrypt.hash(newPassword1, 10);

user.password = password;
await user.save();

res.status(200).send('Password changed');
};

const changeEmail = async (req, res) => {
const { password, newEmail } = req.body;

const user = await userService.findByEmail(req.user.email);
const isPasswordValid = await bcrypt.compare(password, user.password);

if (!isPasswordValid) {
return next(
ApiError.badRequest({ oldPassword: 'Password is not correct' }),
);
}

const confirmToken = uuidv4();

user.confirmToken = confirmToken;
user.confirmTokenExpiry = new Date(Date.now() + 1 * 60 * 60 * 1000);
user.pendingEmail = newEmail;
await user.save();

await emailService.sendConfirmEmail(
newEmail,
confirmToken,
'Confirm changing Email',
'Changing Email address',
);

res.status(200).send('Confirmation email sent');
};

const confirmEmail = async (req, res) => {
const { confirmToken } = req.params;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: User model is used but never imported in profile.controller.js. Line 79 uses User.findOne() which will throw a ReferenceError.

}

const confirmToken = uuidv4();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: In email.service.js, the function sendEmailChanged references undefined variable email instead of the parameter oldEmail. Should be: return send({ email: oldEmail, ... }).

Comment on lines +91 to +92

return next(ApiError.badRequest({ message: 'Token expired' }));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: Missing return before next() on lines 91-92. The code will continue executing after calling next(), potentially causing issues.

Comment thread setup.js
import 'dotenv/config';
import { User } from './src/models/user.js';
import { Token } from './src/models/token.js';
import { client } from './src/utils/db.js';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jwtService is imported but never used in this file.

Comment thread setup.js
@@ -0,0 +1,6 @@
import 'dotenv/config';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The User model lacks an isActive or isActivated field. According to requirements, users should be activated only after email confirmation, but there's no flag to track activation status.

Comment thread setup.js
@@ -0,0 +1,6 @@
import 'dotenv/config';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeName functionality is not implemented as an API endpoint. Requirements state profile should allow changing name.

Comment on lines +14 to +20
});
}

static unAuthorized(errors) {
return new ApiError({
message: 'unauthorized user',
errors,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing next parameter: Functions changePassword, changeEmail, and confirmEmail use next() but don't have it in their parameters. These functions are wrapped with catchError which provides next, but it's not destructured in the function signature.

@@ -0,0 +1,32 @@
export class ApiError extends Error {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import: hash is imported from 'bcrypt' but never used.

export class ApiError extends Error {
constructor({ message, status, errors }) {
super(message);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import: jwtService is imported in profile.controller.js but never used (only userService is needed for getData).

Comment thread src/models/token.js
Comment on lines +5 to +9
export const Token = client.define('token', {
refreshToken: {
type: DataTypes.STRING,
allowNull: false,
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Token model is missing a userId field. The token service tries to query by userId and create tokens with userId, but the model only defines refreshToken. Add userId: { type: DataTypes.INTEGER, allowNull: false } to the model definition.

Comment thread src/models/token.js
Comment on lines +5 to +7
export const Token = client.define('token', {
refreshToken: {
type: DataTypes.STRING,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authMiddleware may incorrectly allow requests when Authorization header exists but has no token (e.g., header is just 'Bearer' with no token value). The split would return ['Bearer', ''] and token would be an empty string which is falsy, so this is actually correct. No issue here.

Comment thread src/index.js
app.get('/', (req, res) => {
res.send('Ok');
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing 404 handler: The requirements explicitly state to display 404 for all routes not defined above (registration, activation, login, logout, password reset, profile). This catch-all should be placed before the error middleware to handle all unmatched routes.

Comment thread src/index.js Outdated
}),
);

app.use(authRouter);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon at end of line.

Comment on lines +17 to +19
html,
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUIREMENT VIOLATION: The User model has activationToken but no boolean flag to track if the user is actually activated. When activationToken is null, the user could be deactivated later without a way to distinguish. Consider adding an isActivated: { type: DataTypes.BOOLEAN, defaultValue: false } field.

<h2>Email changed from ${oldEmail} to ${newEmail}</h2>
`;

return send({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: In sendEmailChanged, line 57 passes email (undefined) instead of oldEmail. The send function will fail because email is not defined in this scope.

Comment on lines +6 to +24
port: process.env.SMTP_PORT,
auth: {
user: process.env.SMTP_USER,
pass: process.env.SMTP_PASSWORD,
},
});

export function send({ email, subject, html }) {
return transporter.sendMail({
to: email,
subject,
html,
});
}

function sendActivationEmail(email, token) {
const href = `${process.env.CLIENT_HOST}/activate/${token}`;

const html = `

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MISSING ENDPOINT: The requirements state Profile page should allow changing name, but there's no /change-name route or handler implemented.

subject: 'Activate',
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MISMATCH: profile.controller.js calls emailService.sendConfirmEmail(newEmail, confirmToken, 'Confirm changing Email', 'Changing Email address') with 4 arguments, but the function only accepts 2 parameters (email, token). The last two arguments are silently ignored.

Comment on lines +7 to +24

profileRouter.get('/', authMiddleware, catchError(profileController.getData));

profileRouter.post(
'/change-email',
authMiddleware,
catchError(profileController.changeEmail),
);
profileRouter.get(
'/change-email/:confirmToken',
catchError(profileController.confirmEmail),
);

profileRouter.post(
'/change-password',
authMiddleware,
catchError(profileController.changePassword),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUIREMENT VIOLATION: The requirements state 'It allows to change a name' on Profile page, but there's no /change-name endpoint defined.

);
profileRouter.get(
'/change-email/:confirmToken',
catchError(profileController.confirmEmail),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CODE QUALITY: Using activationToken === null to determine if user is active is implicit. An explicit isActive boolean field would be clearer and less prone to bugs. Currently, if activationToken is an empty string instead of null, activation checks could fail.

Comment thread src/routes/auth.route.js

authRouter.post('/registration', catchError(authController.register));

authRouter.get(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MISSING REQUIREMENT: The requirements state that to change email, users should 'confirm the new email'. The current implementation only accepts newEmail but doesn't require email confirmation input.

Comment thread src/routes/auth.route.js
authRouter.get(
'/activation/:activationToken',
catchError(authController.activate),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SECURITY NOTE: The email parameter in the send function could be vulnerable to header injection if not validated. However, this may be acceptable for the scope of this task.

Comment thread src/models/user.js
Comment on lines +1 to +33
import { DataTypes } from 'sequelize';
import { client } from '../utils/db.js';

export const User = client.define('user', {
name: {
type: DataTypes.STRING,
allowNull: false,
},
email: {
type: DataTypes.STRING,
allowNull: false,
unique: true,
},
password: {
type: DataTypes.STRING,
allowNull: false,
},
activationToken: {
type: DataTypes.STRING,
},
confirmToken: {
type: DataTypes.STRING,
defaultValue: null,
},
pendingEmail: {
type: DataTypes.STRING,
defaultValue: null,
},
confirmTokenExpiry: {
type: DataTypes.DATE,
defaultValue: null,
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The User model stores the name field correctly, but there's no API endpoint to change it. The requirements specify that Profile page should allow changing the user's name.

Comment thread src/models/user.js
Comment on lines +1 to +23
import { DataTypes } from 'sequelize';
import { client } from '../utils/db.js';

export const User = client.define('user', {
name: {
type: DataTypes.STRING,
allowNull: false,
},
email: {
type: DataTypes.STRING,
allowNull: false,
unique: true,
},
password: {
type: DataTypes.STRING,
allowNull: false,
},
activationToken: {
type: DataTypes.STRING,
},
confirmToken: {
type: DataTypes.STRING,
defaultValue: null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The profile router is missing a route for changing the user's name. According to requirements, the Profile page should allow changing the name.

Comment thread src/models/user.js
Comment on lines +14 to +17
password: {
type: DataTypes.STRING,
allowNull: false,
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /change-email/:confirmToken GET route doesn't have authMiddleware - this is intentional since the token in the URL serves as the authentication mechanism. No issue here.

Comment thread src/services/token.service.js Outdated
Comment on lines +13 to +15

function getByToken(refreshToken) {
return Token.findOne({ where: { refreshToken } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: getByToken is not marked as async but Token.findOne returns a Promise. When this function is awaited elsewhere, it will return a Promise instead of the result.

Comment thread src/services/token.service.js Outdated
Comment on lines +17 to +19

function remove(userId) {
Token.destroy({ where: { userId } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: remove is not marked as async but Token.destroy returns a Promise. This should be awaited in the calling code but the function itself should be async.

Comment thread src/services/token.service.js Outdated
Comment on lines +13 to +15

function getByToken(refreshToken) {
return Token.findOne({ where: { refreshToken } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: findByEmail is not marked as async but User.findOne returns a Promise. When this function is awaited elsewhere (like in auth.controller.js line 80), it will return a Promise instead of the user object.

Comment thread src/services/token.service.js Outdated
Comment on lines +1 to +14
import { Token } from '../models/token.js';

async function save(userId, newToken) {
const token = await Token.findOne({ where: { userId } });

if (!token) {
await Token.create({ userId, refreshToken: newToken });
}

token.refreshToken = newToken;
await token.save();
}

function getByToken(refreshToken) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: catchError passes next to the action (line 3), but profile controller functions like changePassword, changeEmail, and confirmEmail don't destructure next from their parameters. When these functions call next(error), it will throw a ReferenceError because next is not defined.

Comment on lines +5 to +9
expiresIn: '1h',
});

return token;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: When creating a new token on line 6, the return value of Token.create() is not assigned to token. This means token remains null after the if block, and lines 9-10 will fail when trying to access token.refreshToken and token.save() on a null object.

Comment on lines +13 to +15
return jwt.verify(token, process.env.JWT_KEY);
} catch {
return null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: getByToken is an async function (returns a Promise from Sequelize) but is missing await. The caller receives a Promise instead of the actual token object.

Comment on lines +17 to +19
}

function signRefresh(user) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: remove is an async function (Token.destroy returns a Promise) but is missing await. The function should be marked as async and the destroy call should be awaited.

Comment on lines +1 to +38
import jwt from 'jsonwebtoken';

function sign(user) {
const token = jwt.sign(user, process.env.JWT_KEY, {
expiresIn: '1h',
});

return token;
}

function verify(token) {
try {
return jwt.verify(token, process.env.JWT_KEY);
} catch {
return null;
}
}

function signRefresh(user) {
const token = jwt.sign(user, process.env.JWT_REFRESHKEY);

return token;
}

function verifyRefresh(token) {
try {
return jwt.verify(token, process.env.JWT_REFRESHKEY);
} catch {
return null;
}
}

export const jwtService = {
sign,
verify,
signRefresh,
verifyRefresh,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OBSERVATION: The jwt.service.js is implemented correctly. Access tokens expire in 1 hour while refresh tokens don't expire (they are stored in the database and can be revoked). This is a reasonable implementation.

Comment on lines +1 to +38
import jwt from 'jsonwebtoken';

function sign(user) {
const token = jwt.sign(user, process.env.JWT_KEY, {
expiresIn: '1h',
});

return token;
}

function verify(token) {
try {
return jwt.verify(token, process.env.JWT_KEY);
} catch {
return null;
}
}

function signRefresh(user) {
const token = jwt.sign(user, process.env.JWT_REFRESHKEY);

return token;
}

function verifyRefresh(token) {
try {
return jwt.verify(token, process.env.JWT_REFRESHKEY);
} catch {
return null;
}
}

export const jwtService = {
sign,
verify,
signRefresh,
verifyRefresh,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OBSERVATION: The user.service.js correctly implements the register function with email uniqueness check and activation token generation. The normalize function correctly only includes id and email for JWT payload (not including password).

Comment on lines +1 to +8
import jwt from 'jsonwebtoken';

function sign(user) {
const token = jwt.sign(user, process.env.JWT_KEY, {
expiresIn: '1h',
});

return token;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OBSERVATION: The catchError utility correctly passes next to the action function, which explains how next() works in profile.controller.js even though those functions don't have next in their Express-provided parameters.

Comment on lines +17 to +18

async function register(name, email, password) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remove function on line 18 does not await the Token.destroy() call. This async operation should be awaited to ensure the token is properly removed from the database before the logout response is sent.

Comment thread src/services/user.service.js Outdated
Comment on lines +13 to +15

function findByEmail(email) {
return User.findOne({ where: { email } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getByToken function returns a Promise but is not marked as async. While technically it returns the Promise directly which works, marking it as async would be more consistent with other functions in this file.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

The code has made significant progress fixing previous critical bugs (uuidv4 import, await on bcrypt.compare, next parameter, User import, token service upsert), but three critical functional requirements remain unimplemented: (1) CHECKLIST ITEM #8 - Login in src/controllers/auth.controller.js doesn't verify user.activationToken === null. Add if (user.activationToken) { throw ApiError.badRequest('Please activate your email first'); } after password validation around line 82. (2) CHECKLIST ITEM #6 - The activate function returns res.send(user) instead of redirecting. Replace with res.redirect(process.env.CLIENT_HOST + '/profile'). (3) CHECKLIST ITEM #9 - Login should redirect to Profile after successful login, not just return user data and token. Note: The ESLint .cjs configuration issue is a tooling problem, not student code, and won't block approval once the functional requirements above are fixed.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +78 to +88

const login = async (req, res) => {
const { email, password } = req.body;

const user = await userService.findByEmail(email);
const isPasswordValid = await bcrypt.compare(password, user.password);

if (!user || !isPasswordValid) {
throw ApiError.badRequest('Email or password is incorrect');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #8: Login doesn't check if user is activated. If user is not active, you should ask them to activate their email. Currently the login allows inactive users to login.

Comment on lines +78 to +88

const login = async (req, res) => {
const { email, password } = req.body;

const user = await userService.findByEmail(email);
const isPasswordValid = await bcrypt.compare(password, user.password);

if (!user || !isPasswordValid) {
throw ApiError.badRequest('Email or password is incorrect');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #9: Login should redirect to Profile page after successful login. Currently it only returns user data and accessToken without redirect.

Comment on lines +78 to +89
user.pendingEmail = newEmail;
await user.save();

await emailService.sendConfirmEmail(newEmail, confirmToken);

res.status(200).send('Confirmation email sent');
};

const confirmEmail = async (req, res, next) => {
const { confirmToken } = req.params;

const user = await User.findOne({ where: { confirmToken } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #8: Missing activation check during login. According to requirements: 'If user is not active, ask them to activate their email'. Currently, users who haven't activated their account can still log in. Add a check: if user.activationToken is not null, throw an ApiError.badRequest with a message asking the user to activate their email.

Comment on lines +62 to +76
const changeEmail = async (req, res, next) => {
const { password, newEmail } = req.body;

const user = await userService.findByEmail(req.user.email);
const isPasswordValid = await bcrypt.compare(password, user.password);

if (!isPasswordValid) {
return next(
ApiError.badRequest({ oldPassword: 'Password is not correct' }),
);
}

const confirmToken = uuidv4();

user.confirmToken = confirmToken;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #6: The activate function sends user data but should redirect to Profile page. According to requirements: 'redirect to Profile after the activation'. Use res.redirect(process.env.CLIENT_HOST + '/profile') instead of res.send(user).

Comment on lines +81 to +82
sendActivationEmail,
sendConfirmEmail,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #8: Login doesn't verify if user.activationToken is set. Add after line 81: if (user.activationToken) { throw ApiError.badRequest('Please activate your email first'); }

Comment on lines +62 to +76
}

function sendResetPassword(email, token) {
const href = `${process.env.CLIENT_HOST}/reset-password/${token}`;

const html = `
<h1>Confirm password changing</h1>
<a href="${href}">${href}</a>
`;

return send({
email,
html,
subject: 'Password changing',
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #6: After successful activation, should redirect to Profile. Replace res.send(user) with res.redirect(process.env.CLIENT_HOST + '/profile').

Comment thread src/models/user.js
@@ -0,0 +1,33 @@
import { DataTypes } from 'sequelize';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKLIST ITEM #8 is still missing: The login function (auth.controller.js) does not check if user.activationToken is set. If a user hasn't activated their email, they should receive an error asking them to activate. Add this check before or after line 82 in auth.controller.js.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Here is my approval for your solution, which successfully implements all core authentication requirements including registration, activation, login, password reset, and profile management. Your implementation correctly handles email verification, token generation, and protected routes.

However, there's one remaining bug to fix before production: The logout function in both src/controllers/profile.controller.js and src/controllers/auth.controller.js uses res.sendStatus(204).redirect(), which is incorrect because sendStatus(204) ends the response immediately, preventing the redirect from executing. Fix: In profile.controller.js, remove sendStatus(204) entirely and just use res.redirect(process.env.CLIENT_HOST + '/login');. In auth.controller.js, use res.status(204).redirect(process.env.CLIENT_HOST + '/login'); instead of chaining methods.

Additionally, consider adding authMiddleware to the logout endpoint in src/routes/auth.route.js since logout should only be available for authenticated users. The ESLint configuration issue with .eslintrc.js is a tooling problem, not related to your code.

You've made excellent progress fixing the three critical issues from the previous review. Keep up the great work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

user.email = user.pendingEmail;
user.pendingEmail = null;
user.confirmToken = null;
user.confirmTokenExpiry = null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chaining makes redirect unreachable - sendStatus(204) returns the res object, so redirect never executes. The requirement is to 'Redirect to login after logging out'. Change to just res.redirect(process.env.CLIENT_HOST + '/login');

Comment thread src/controllers/auth.controller.js Outdated
Comment on lines +108 to +109

res.sendStatus(204).redirect(process.env.CLIENT_HOST + '/login');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

res.sendStatus(204) already ends the response, so chaining .redirect() after it won't execute. Separate the status and redirect: res.status(204).redirect(process.env.CLIENT_HOST + '/login');

Comment thread src/models/user.js
unique: true,
},
password: {
type: DataTypes.STRING,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logout endpoint is mapped to POST method in auth.route.js (line 15), but the description requires logout to be available only for authenticated users. There's no authMiddleware protecting this route like there is on profile routes. Consider whether POST /logout should have authentication.

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress, but you need to pass all tests before requesting the review

@yahohulia

Copy link
Copy Markdown
Author

This problem hasn't been resolved, but Luk said it doesn't affect the operation

@yahohulia yahohulia requested a review from brespect June 15, 2026 12:15

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw, but the requirements of any technical or business task are much more valuable than AI suggestions. Luke is just an auxiliary tool that simplifies the review

@yahohulia

Copy link
Copy Markdown
Author

But the main reason for this bug is that the validation is done by ESLint via mateTest, which is why it throws an error; and switching from Module to CommonJS, in my opinion, is a time-consuming and unnecessary task.

@yahohulia yahohulia requested a review from brespect June 17, 2026 13:23

@Anton-Kuchmasov Anton-Kuchmasov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

Left some comments below - might be helpful in your further tasks

Comment thread src/controllers/profile.controller.js Outdated
);
}

if (newPassword1.trim() !== newPassword2.trim()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad naming - newPassword, confirmedPassword should be better

Comment thread src/controllers/profile.controller.js Outdated
const confirmToken = uuidv4();

user.confirmToken = confirmToken;
user.confirmTokenExpiry = new Date(Date.now() + 1 * 60 * 60 * 1000);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TTL should be saved as a constant

user.pendingEmail = newEmail;
await user.save();

await emailService.sendConfirmEmail(newEmail, confirmToken);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if something went wrong with confirmation email sending, you should handle this - no need to respond 200 in this case, it's crucial issue

use try-catch statement here

Comment thread src/controllers/profile.controller.js Outdated
const user = await User.findOne({ where: { confirmToken } });

if (!user) {
throw ApiError.notFound();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some clarification here - e.g. errorType or errorMessage fields

Comment thread src/routes/oauth.route.js Outdated
await tokenService.save(normalizedUser.id, refreshToken);

res.cookie('refreshToken', refreshToken, {
maxAge: 30 * 24 * 60 * 60 * 1000,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here - it's easy to read 30 days TTL here, but it should be saved as a const

Comment thread src/routes/oauth.route.js
});

res.redirect(
`${process.env.CLIENT_HOST}/oauth?accessToken=${accessToken}&user=${encodeURIComponent(JSON.stringify(normalizedUser))}`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsave practice, it's better to use URL Constructor here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants